task: support multiple waiters in WaitQueue#1114
Conversation
mvanhorn
requested review from
00xc,
joergroedel,
msft-jlange,
peterfang and
stefano-garzarella
as code owners
|
Unfortunately, this design won't work because it requires allocation of memory when entering a wait state (as a result of Please reconsider your design to implement a queue without requiring memory allocation. |
| /// The caller must ensure that `task` is already a member of this task | ||
| /// list. | ||
| unsafe fn terminate(&mut self, task: TaskPointer) -> Option<TaskPointer> { | ||
| unsafe fn terminate(&mut self, task: TaskPointer) -> alloc::vec::Vec<TaskPointer> { |
There was a problem hiding this comment.
As a general pattern, we prefer a use statement in the file so that types in declarations do not require namespace decoration. So you would want use alloc::vec::Vec at the top of the file and then this just becomes Vec<TaskPointer>.
| if let Some(task) = target { | ||
| wait_for_termination(task); | ||
| } | ||
| MULTI_WAITER_COUNTER.fetch_add(10, Ordering::Relaxed); |
There was a problem hiding this comment.
What does 10 mean? Is it a magic value?
| self.waiter.take() | ||
| /// Wake all waiting tasks. Returns the drained list of waiters so the | ||
| /// caller can schedule each one. The drain itself does not allocate. | ||
| pub fn wakeup(&mut self) -> Vec<TaskPointer> { |
There was a problem hiding this comment.
In general, some wake operations will want to wake the entire queue and some operations will only want to wake the head of the queue. Waiting for task termination clearly falls into former category but turnstile-type wait operations can only wake one at a time. While it's not necessary to implement this now, it would be nice to anticipate this by defining a parameter to this function (e.g. wake_all: bool) that permits both types. If you don't want to implement both now, then you can just assert!(wake_all) for now and make it a problem for the future, but defining this now at least requires callers to be conscious of the type of wakeup so that we don't have to reconsider the behavior of entire code base when this single wakeup is finally implemented.
msft-jlange
left a comment
msft-jlange
left a comment
There was a problem hiding this comment.
Please reimplement as described without using memory allocation.
I second this, the prefered data structure to use here is a linked list from |
|
Reworked in d2626cd per both of your points: the Vec is gone. The waiters queue is now an intrusive linked list from intrusive_collections, with the LinkedListAtomicLink embedded in Task next to the existing runlist link, so wait_for_event() performs no allocation and the allocator can safely participate in waits. wakeup() takes the list and hands it back to the scheduler for wake-up. cargo check for the svsm kernel target passes locally. |
|
@mvanhorn please rebase since there are conflicts and also check all comments and resolve ones fixed or explain why they are not fixed. |
mvanhorn
force-pushed
the
fix/1104-waitqueue-multiple-waiters
branch
from
d2626cd to
7d11685
Compare
|
Rebased onto main. There was one conflict in Still working through @msft-jlange's review comments (the |
Thanks for continuing to work through these. I haven't had a chance to look at your updates yet, but your description is certainly headed in the right direction. One other issue - we require each commit in a PR to be separately stable, and your updates have not resolved the |
|
All four items addressed as of 8fd94da:
Verified: cargo check and clippy clean for x86_64-unknown-none, fmt clean. The multi-waiter test is test_in_svsm, so it runs in-guest via CI. |
| impl Default for WaitQueue { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not needed after #1162 and should be removed.
| ) | ||
| .expect("Failed to start waiter 2"); | ||
|
|
||
| // Wait for the target task from this task too, ensuring it has |
There was a problem hiding this comment.
Nothing here ensures that waiter 1 and waiter 2 enter the wait state while the target task is still executing so your tests cannot prove that the waiters actually join the wait queue. In fact, because your target task runs on the same CPU that the test function runs on, it is virtually guaranteed that the target task will terminate before either waiter thread is even created, so it is overwhelmingly likely that there is no wait queue testing occurring here. A proper test would require the following sequence:
- Change the target task to set affinity to some processor other than the originating processor (use the same logic that is in
empty_task()). - Add a barrier to the target task so that it will spin-wait for the barrier to be signaled until it proceeds to termination.
- Signal the barrier from the test task after both waiters have been created but before the test task performs its own wait. This will guarantee that both waiter 1 and waiter 2 have entered wait states and will also guarantee that the target task has not terminated before both waiters have entered their wait states.
|
@mvanhorn More actions are still required.
|
mvanhorn
force-pushed
the
fix/1104-waitqueue-multiple-waiters
branch
from
8fd94da to
379df30
Compare
| if let Some(wake_task) = wakeup { | ||
| // Wake all tasks that were waiting for this task to terminate, scheduling | ||
| // each on the current runqueue. | ||
| for wake_task in wakeup { |
There was a problem hiding this comment.
I think this is subject to race conditions. Once a task has been woken, it can immediately attempt to enter another wait state. However, for the duration of this loop, the task's wait list link is still inserted into the wakeup list here. That means that an attempt by the task to enter a new wait state will panic when it tries to insert itself into a different wait queue. I believe it is necessary to ensure that the task being woken is removed from the local wakeup list before the wakeup can be attempted.
WaitQueue previously supported only a single waiter; a second waiter would overwrite the first and strand it forever. Store waiters in a queue so every waiter is woken. Fixes coconut-svsm#1104 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Review feedback addressed and amended into the signed commit (9166b00, history kept squashed per repo convention). Verified locally: targeted x86_64 svsm build and clippy pass offline. |
mvanhorn
force-pushed
the
fix/1104-waitqueue-multiple-waiters
branch
from
379df30 to
9166b00
Compare
4 participants
Summary
WaitQueueusedOption<TaskPointer>to hold at most one waiter and panicked if a second task calledwait_for_eventwhile one was already queued. This made it impossible for multiple tasks to callwait_for_terminationon the same target task simultaneously.Why this matters
As described in #1104, the
wait_for_terminationAPI is the natural way to join a task, but any scenario where two tasks join the same target — e.g., a parent and a monitor both waiting on a worker — triggers an unconditional panic. The fix unlocks that usage pattern without any heap allocation in the wake path.Changes
kernel/src/task/waiting.rs: Replacewaiter: Option<TaskPointer>withwaiters: Vec<TaskPointer>. Remove theassert!(self.waiter.is_none())guard.wait_for_eventnow pushes onto the vec;wakeupdrains it (alloc-free at wake time) and returns the full list.kernel/src/task/tasks.rs:set_task_terminatednow returnsVec<TaskPointer>instead ofOption<TaskPointer>.kernel/src/task/schedule.rs:TaskList::terminatereturnsVec<TaskPointer>;current_task_terminatediterates over all returned waiters and callsenqueue_taskfor each. Addstest_wait_for_termination_multiple_waiters: two tasks each callwait_for_terminationon a third task; both must resume after it terminates.Fixes #1104
Signed-off-by: Matt Van Horn mvanhorn@gmail.com